Fix issue 2085 #2136

camilasan · 2020-06-29T18:48:05Z

Update systray behavior and context menu:

left click brings up the new QtQuick based dialogs on all latforms
right click brings up the new QtQuick based dialog on Mac OS only
right click brings up a context menu on all other platforms than Mac OS
"Quit Nextcloud" => "Quit"
Add "Open Main Dialog" option: add new slot to show the main dialog and fix
open settings slot to correctly show the settings.

Fix #2085

to do

Test on Mac OS.
I always run clang now so it found some issues and it fixed it... I can move that last commit to somewhere else if anyone is against it being in the same PR.

er-vin · 2020-06-30T16:38:03Z

src/libsync/creds/httpcredentials.cpp

@@ -114,8 +114,8 @@ HttpCredentials::HttpCredentials(const QString &user, const QString &password, c
    , _ready(true)
    , _clientSslKey(key)
    , _clientSslCertificate(certificate)
-    , _keychainMigration(false)
-    , _retryOnKeyChainError(false)
+    , 


Hm, there's a couple of weird formatting happening. Let's keep the clang-tidy fix aside for now. I need to fix the CI integration first anyway.

er-vin · 2020-06-30T16:47:24Z

src/gui/owncloudgui.cpp

+    bool macos = false;
+#ifdef Q_OS_MAC
+    macos = true;
+#endif
+
+    if(reason == QSystemTrayIcon::Trigger ||
+            (macos && reason == QSystemTrayIcon::Context)){


Not a huge fan of that construct, I think I'd prefer either something like:

#ifdef Q_OS_MAC if (reason == QSystemTrayIcon::Trigger || reason == QSystemTryIcon::Context) { #else if (reason == QSystemTrayIcon::Trigger) { #endif

or:

#ifdef Q_OS_MAC constexpr auto macos = true; #else constexpr auto macos = false; #endif if (reason == QSystemTrayIcon::Trigger || (macos && reason == QSystemTryIcon::Context)) {

Also somehow you changed the coding style of the if (space got removed before the opening parenthesis and after the closing one).

Oh BTW... I hoped we didn't even need to do this... out of curiosity, what happens if you unconditionally leave Context in the if? The context menu doesn't show up anymore on Linux and Windows?

Oh BTW... I hoped we didn't even need to do this... out of curiosity, what happens if you unconditionally leave Context in the if? The context menu doesn't show up anymore on Linux and Windows?

You are right I don't need to check for the context click. I just need to test this on mac os now that I have access to one.

After you add your new account the menu is displayed in a weird position (more to the left of the systray away from the nc icon instead of near the nc systray icon)

Sorry, I think I have been unclear earlier.

What I meant was: I wonder if we even need to change anything at all in slotTrayClicked!

Indeed, if we're on Mac we test for the QSystemTrayIcon::Context reason and we didn't call setContextMenu so we'll display the main dialog in both left and right click. If we're not on Mac, we called setContextMenu... then I wonder if we get QSystemTrayIcon::Context delivered at all. If not, then we don't need to change slotTrayClicked otherwise... well, we'll need to do something in slotTrayClicked indeed.

Removed it and it worked too.

er-vin · 2020-06-30T18:35:43Z

src/gui/systray.cpp

@@ -79,6 +80,14 @@ Systray::Systray()
        }
    );

+#ifndef Q_OS_MAC
+      auto contextMenu = new QMenu();


Indentation seems busted in that block.

Also the request from @jancborchardt was to have: "Open Main Dialog", "Settings" and "Quit" in that menu.

What is the main dialog? Is it the new systray menu or the mains window with the settings and network?

desktop/src/gui/application.cpp

Line 667 in be4ba03

_gui->slotOpenMainDialog();

I would say this call after the user created his account needs to open the window with the settings not the new systray menu.

What is the main dialog? Is it the new systray menu or the mains window with the settings and network?

It is the new window you get when clicking on the systray. We call that the "main dialog" now, the other one being the "settings dialog".

desktop/src/gui/application.cpp

Line 667 in be4ba03

_gui->slotOpenMainDialog();

I would say this call after the user created his account needs to open the window with the settings not the new systray menu.

Dunno... I don't think I mind either way. :-)

Yup. Undid it.

Also the request from @jancborchardt was to have: "Open Main Dialog", "Settings" and "Quit" in that menu

Sorry, I just wrote that in the chat based on what the things should do, not the exact wording. It should of course reflect the wording we use in the main dialog:

Open main dialog (only first word capitalized

Settings

Quit Nextcloud

Since we had the point brought up in the chat as well, we could take the opportunity to change it to "Exit Nextcloud" instead, here as well as in the main dialog.

er-vin · 2020-07-01T19:00:50Z

src/gui/owncloudgui.cpp

+#endif
+
+    // for mac os right and left click trigger the same behavior
+    if (reason == QSystemTrayIcon::Trigger || macos){


If we still have a change in here (see my other comment), it really should be || (macos && reason == QSystemTrayIcon::Trigger) and not just || macos.

Removed it.

er-vin · 2020-07-01T19:01:55Z

src/gui/owncloudgui.cpp

+    _settingsDialog->showSettingsPage();
+}
+
+void ownCloudGui::slotShowMainDialog()


This is wrongly named, this really shows the settings dialog not the main dialog.

misch7 · 2020-07-02T00:13:17Z

src/gui/owncloudgui.cpp

 void ownCloudGui::slotShowSettings()
 {
-    if (_settingsDialog.isNull()) {
-        _settingsDialog = new SettingsDialog(this);
-        _settingsDialog->setAttribute(Qt::WA_DeleteOnClose, true);
-        _settingsDialog->show();
+    if (!AccountManager::instance()->accounts().isEmpty()) {
+        if (_settingsDialog.isNull()) {
+            _settingsDialog = new SettingsDialog(this);
+            _settingsDialog->setAttribute(Qt::WA_DeleteOnClose, true);
+            _settingsDialog->show();
+        }
+        raiseDialog(_settingsDialog.data());
+    } else {
+        slotNewAccountWizard();
    }
-    raiseDialog(_settingsDialog.data());
 }


Tested on Windows and works fine except when no accounts are configured, then the Settings option only brings up the account wizard.

Admittedly it's a side-effect this fix wasn't supposed to tackle... but to be fair, isn't it what we'd want nowadays? Because you can't add accounts from the settings dialog anymore it's triggered from the main dialog. Ah but then you wouldn't be able to set a proxy before creating an account... that could be a problem I guess.

Maybe that chunk should be reverted then?

src/gui/owncloudgui.cpp

er-vin · 2020-07-02T06:13:25Z

src/gui/owncloudgui.cpp

 void ownCloudGui::slotShowSettings()
 {
-    if (_settingsDialog.isNull()) {
-        _settingsDialog = new SettingsDialog(this);
-        _settingsDialog->setAttribute(Qt::WA_DeleteOnClose, true);
-        _settingsDialog->show();
+    if (!AccountManager::instance()->accounts().isEmpty()) {
+        if (_settingsDialog.isNull()) {
+            _settingsDialog = new SettingsDialog(this);
+            _settingsDialog->setAttribute(Qt::WA_DeleteOnClose, true);
+            _settingsDialog->show();
+        }
+        raiseDialog(_settingsDialog.data());
+    } else {
+        slotNewAccountWizard();
    }
-    raiseDialog(_settingsDialog.data());
 }


Admittedly it's a side-effect this fix wasn't supposed to tackle... but to be fair, isn't it what we'd want nowadays? Because you can't add accounts from the settings dialog anymore it's triggered from the main dialog. Ah but then you wouldn't be able to set a proxy before creating an account... that could be a problem I guess.

Maybe that chunk should be reverted then?

er-vin · 2020-07-02T07:33:23Z

src/gui/owncloudgui.cpp

@@ -145,7 +148,7 @@ void ownCloudGui::slotOpenSettingsDialog()
    // if account is set up, start the configuration wizard.
    if (!AccountManager::instance()->accounts().isEmpty()) {
        if (_settingsDialog.isNull() || QApplication::activeWindow() != _settingsDialog) {
-            slotShowSettings();
+            slotOpenMainDialog();


I'm confused, shouldn't that be slotShowSettings()?

I mean the check on the if is all about the _settingsDialog being active so it's surprising to then go and open something else.

er-vin · 2020-07-02T16:58:18Z

src/gui/owncloudgui.cpp

@@ -85,6 +85,9 @@ ownCloudGui::ownCloudGui(Application *parent)
    connect(_tray.data(), &Systray::openHelp,
        this, &ownCloudGui::slotHelp);

+    connect(_tray.data(), &Systray::openMainDialog,
+        this, &ownCloudGui::slotOpenSettingsDialog);


You meant slotOpenMainDialog here, didn't you? Otherwise it'd show the settings dialog when we click on "Open main dialog" I think.

I know what I wanted with this: to check if the user was logged in or not. That is what slotOpenSettingsDialog does... and that is why I had added that code to slotShowSettings... I guess I should just display the main dialog.

er-vin · 2020-07-02T16:59:26Z

src/gui/systray.cpp

@@ -93,6 +102,8 @@ void Systray::create()
            _trayEngine->rootContext()->setContextProperty("activityModel", UserModel::instance()->currentActivityModel());
        }
        _trayEngine->load(QStringLiteral("qrc:/qml/src/gui/tray/Window.qml"));
+
+        _trayEngine->rootContext()->setContextProperty("appNameGUI", Theme::instance()->appNameGUI());


Don't change it now. FYI we'll have to change that later on, context properties are deprecated, that's why we got Nico's patches to switch to QML singletons. That can wait.

Update systray behavior and context menu: - left click brings up the new QtQuick based dialogs on all latforms - right click brings up the new QtQuick based dialog on Mac OS only - right click brings up a context menu on all other platforms than Mac OS - "Quit Nextcloud" => "Exit Nextcloud" - Add "Open main dialog" option. Signed-off-by: Camila <[email protected]>

The offending part was reverted, so good to go now

sbeyer · 2020-07-06T13:23:26Z

I got to test this now and this feels just right in KDE now! Thanks for this!

camilasan marked this pull request as draft June 29, 2020 18:48

camilasan added this to the 2.7 🌟 UI improvements milestone Jun 29, 2020

er-vin reviewed Jun 30, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch 2 times, most recently from 2532133 to a8306d1 Compare June 30, 2020 18:09

er-vin reviewed Jun 30, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch from a8306d1 to 46054b4 Compare July 1, 2020 17:23

camilasan requested review from jancborchardt and misch7 July 1, 2020 17:48

camilasan marked this pull request as ready for review July 1, 2020 17:53

camilasan force-pushed the fix-issue-2085 branch from 2868c54 to 0aa9e54 Compare July 1, 2020 18:36

er-vin reviewed Jul 1, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch 2 times, most recently from b456025 to 140f50f Compare July 1, 2020 20:44

misch7 previously requested changes Jul 2, 2020

View reviewed changes

er-vin reviewed Jul 2, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch 2 times, most recently from 0c9b464 to 3c5eec1 Compare July 2, 2020 07:11

er-vin reviewed Jul 2, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch from 3c5eec1 to 082c87c Compare July 2, 2020 16:01

er-vin requested changes Jul 2, 2020

View reviewed changes

camilasan force-pushed the fix-issue-2085 branch from 082c87c to 00f1e12 Compare July 2, 2020 17:06

er-vin approved these changes Jul 2, 2020

View reviewed changes

er-vin merged commit fdc1604 into master Jul 2, 2020

er-vin deleted the fix-issue-2085 branch July 2, 2020 17:21

misch7 mentioned this pull request Jul 4, 2020

macOS: Display Dock icon and use its context menu #2155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 2085 #2136

Fix issue 2085 #2136

camilasan commented Jun 29, 2020 •

edited by er-vin

Loading

er-vin Jun 30, 2020

er-vin Jun 30, 2020

er-vin Jun 30, 2020

camilasan Jun 30, 2020 •

edited

Loading

camilasan Jul 1, 2020

er-vin Jul 1, 2020

camilasan Jul 1, 2020

er-vin Jun 30, 2020

camilasan Jul 1, 2020 •

edited

Loading

er-vin Jul 1, 2020

camilasan Jul 1, 2020

jancborchardt Jul 2, 2020

camilasan Jul 2, 2020

er-vin Jul 1, 2020

camilasan Jul 1, 2020

er-vin Jul 1, 2020

camilasan Jul 1, 2020

misch7 Jul 2, 2020

er-vin Jul 2, 2020

camilasan Jul 2, 2020

er-vin Jul 2, 2020

er-vin Jul 2, 2020

camilasan Jul 2, 2020

er-vin Jul 2, 2020

camilasan Jul 2, 2020

er-vin Jul 2, 2020

sbeyer commented Jul 6, 2020

Fix issue 2085 #2136

Fix issue 2085 #2136

Conversation

camilasan commented Jun 29, 2020 • edited by er-vin Loading

to do

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilasan Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilasan Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbeyer commented Jul 6, 2020

camilasan commented Jun 29, 2020 •

edited by er-vin

Loading

camilasan Jun 30, 2020 •

edited

Loading

camilasan Jul 1, 2020 •

edited

Loading